-
Notifications
You must be signed in to change notification settings - Fork 414
[Custom Transactions] Add TxBuilder
trait, support fixed additional outputs
#3775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @carlaKC as a reviewer! |
c245ca1
to
7fc2092
Compare
7fc2092
to
6aa0848
Compare
fa3ea56
to
14b83a3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3775 +/- ##
==========================================
+ Coverage 89.53% 90.37% +0.84%
==========================================
Files 157 160 +3
Lines 125310 133578 +8268
Branches 125310 133578 +8268
==========================================
+ Hits 112192 120721 +8529
+ Misses 10429 10231 -198
+ Partials 2689 2626 -63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a reasonable API on first pass! Will have something smarter to say once I've rebased on this 👌
🔔 4th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 5th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 6th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 7th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 8th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 9th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 10th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 11th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
🔔 12th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I assume this is kinda blocked until we land more of 0FC?
lightning/src/ln/chan_utils.rs
Outdated
@@ -622,6 +622,21 @@ impl HTLCOutputInCommitment { | |||
&& self.cltv_expiry == other.cltv_expiry | |||
&& self.payment_hash == other.payment_hash | |||
} | |||
|
|||
pub(crate) fn is_dust(&self, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want the interface to decide this, shouldn't it just be a new flag in HTLCOutputInCommitment
rather than a method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appended a commit below that makes this a flag - hopefully I've understood you :) Assuming for now we don't let users of TxBuilder
decide which HTLCs are dust-vs-nondust.
let mut value_to_self_msat_offset = 0; | ||
|
||
let feerate_per_kw = feerate_per_kw.unwrap_or_else(|| self.get_commitment_feerate(funding, generated_by_local)); | ||
|
||
for htlc in self.pending_inbound_htlcs.iter() { | ||
if htlc.state.included_in_commitment(generated_by_local) { | ||
if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume in the future this will be something the TxBuilder
decides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LN spec is moving in a direction where whether a HTLC is dust is solely a function of its amount and the dust_limit_satoshis
p2p setting - so I thought we could let channel make that decision. Could we require all future HTLC-transactions (including customized ones) to pay exogenous fees ? I find them so superior so I thought this was fair.
I have an alternative branch where TxBuilder
makes that decision - but it got thorny quite fast (see get_pending_htlc_stats, get_available_balances_for_scope, next_local/remote_commit_tx_fee_sat
), but happy to revisit this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LN spec is moving in a direction where whether a HTLC is dust is solely a function of its amount and the dust_limit_satoshis p2p setting - so I thought we could let channel make that decision. Could we require all future HTLC-transactions (including customized ones) to pay exogenous fees ? I find them so superior so I thought this was fair.
Hmm, but we also want to be able to sign for existing channel types, presumably, not only anchor-0FC? Also, I could see a future change to the spec removing the dust_limit_satoshis
setting and just fixing it to the network level, which would also make it depend on the scriptPubKey type.
I have an alternative branch where TxBuilder makes that decision - but it got thorny quite fast (see get_pending_htlc_stats, get_available_balances_for_scope, next_local/remote_commit_tx_fee_sat), but happy to revisit this.
Yea, I imagine it will require a larger refactor such that those methods can be implemented as a function of build_commitment_stats
, but sadly I just don't think its a reasonable assumption to make. Of course that doesn't mean we have to do that now, we can certainly start with that assumption and relax it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but we also want to be able to sign for existing channel types, presumably, not only anchor-0FC? Also, I could see a future change to the spec removing the dust_limit_satoshis setting and just fixing it to the network level, which would also make it depend on the scriptPubKey type.
Certainly - but currently I assume this: "only allow anchor, 0FC channels to be customized, and not static only channels."
I do see your point about removing from channel all branches on channel type. IIUC this is not a blocker for this PR let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I imagine it will require a larger refactor such that those methods can be implemented as a function of build_commitment_stats, but sadly I just don't think its a reasonable assumption to make. Of course that doesn't mean we have to do that now, we can certainly start with that assumption and relax it later.
Sounds good I'll push a refactor as a follow-up to this PR is that ok ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly - but currently I assume this: "only allow anchor, 0FC channels to be customized, and not static only channels."
Hmm, yea, would be nice to just have a "commitment transactions are not the responsibility of channel.rs
" goal. Doesn't have to get there now, but eventually would be nice to not have channel.rs
have any concept for commitment transactions, only stats on them.
Yes would love to merge this as part of the 0FC PR sequence ! Also would like to merge #3855 first, to make sure whatever API we settle on here fits in nicely with a splicing world; in that world for a single set of included HTLCs based on their state, we generate multiple commitments, so this is relevant to this PR here. |
Oh! Okay, I thought we were gonna do 0FC first, but happy to do this first. |
What's left to get this out of draft, then? Want to make quick progress on 0FC. |
14b83a3
to
2541f45
Compare
9dba3c2
to
2f19710
Compare
0071eff
to
5d6baeb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments, do you want to pick a second reviewer or should the bot do it?
lightning/src/ln/channel.rs
Outdated
if match update_state { | ||
// Note that these match the inclusion criteria when scanning | ||
// pending_inbound_htlcs below. | ||
FeeUpdateState::RemoteAnnounced => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that its super critical in this case, but when moving code, please move in a separate commit from rustfmt
, such that git show --color-moved --color-moved-ws=ignore-space-change
can highlight the move.
lightning/src/ln/channel.rs
Outdated
remote_balance_before_fee_anchors_msat: u64, // remote balance before fees and anchors *not* considering dust limits | ||
#[derive(Debug, PartialEq)] | ||
pub(crate) struct CommitmentStats { | ||
pub(crate) total_fee_sat: u64, // the total fee included in the transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanna go ahead and make the comments documentation?
lightning/src/ln/channel.rs
Outdated
#[derive(Debug, PartialEq)] | ||
pub(crate) struct CommitmentStats { | ||
pub(crate) total_fee_sat: u64, // the total fee included in the transaction | ||
pub(crate) local_balance_before_fee_msat: u64, // local balance before fees and anchors *not* considering dust limits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are now wrong, as the fields are now post-anchors.
lightning/src/ln/channel.rs
Outdated
@@ -4146,31 +4172,21 @@ where | |||
{ | |||
let remote_commit_tx_fee_msat = if funding.is_outbound() { 0 } else { | |||
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered); | |||
self.next_remote_commit_tx_fee_msat(funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations | |||
self.next_remote_commit_tx_fee_msat(&funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like unnecessary diff to add the & before funding here and below.
lightning/src/ln/channel.rs
Outdated
// `Some(())` is for the fee spike buffer we keep for the remote. This deviates from | ||
// the spec because the fee spike buffer requirement doesn't exist on the receiver's | ||
// side, only on the sender's. Note that with anchor outputs we are no longer as | ||
// sensitive to fee spikes, so we need to account for them. | ||
// | ||
// A `None` `HTLCCandidate` is used as in this case because we're already accounting for | ||
// the incoming HTLC as it has been fully committed by both sides. | ||
let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(funding, None, Some(())); | ||
let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(&funding, None, Some(())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include the extra dust-buffer HTLC in the build_commitment_stats
call, above, to match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion. I looked into this, here's the major difference between the two paths:
next_remote_commit_tx_fee_msat
will set the dust threshold for HTLCs based oncontext.feerate_per_kw
- the number of HTLCs passed to
build_commitment_stats
above come fromget_pending_htlc_stats
, which itself will set the HTLC dust threshold according tocontext.get_dust_buffer_feerate(outbound_feerate_update)
Therefore number of nondust HTLCs will differ.
Therefore, commit_tx_fee_sat
will differ.
lightning/src/ln/chan_utils.rs
Outdated
cltv_expiry: cltv_expiry.0.unwrap(), | ||
payment_hash: payment_hash.0.unwrap(), | ||
transaction_output_index, | ||
is_dust: transaction_output_index.is_none(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, should this just be an fn
that checks if the output index is none? Would imply passing something other than HTLCOutputInCommitment
to build_commitment_transaction
(as at that point it doesn't have an output index), but IMO that's a cleaner API anyway.
Thanks I pinged Carla how does that sound ? |
5d6baeb
to
3f98b95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started review yesterday so please ignore any outdated comments!
lightning/src/ln/channel.rs
Outdated
local_balance_before_fee_msat: u64, // local balance before fees and anchors *not* considering dust limits | ||
remote_balance_before_fee_msat: u64, // remote balance before fees and anchors *not* considering dust limits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment remove "and anchors" from comments
lightning/src/sign/tx_builder.rs
Outdated
value_to_self_after_htlcs: u64, value_to_remote_after_htlcs: u64, | ||
channel_type: &ChannelTypeFeatures, | ||
) -> CommitmentStats { | ||
let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, nondust_htlc_count, channel_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a goal be to allow implementations of TxBuilder
to pick their own feerate (ie, not the value that commit_tx_fee_sat
would set?
If yes, transaction_fee_satoshis
would be wrong in get_available_balances. Doesn't look like this is actually used, but it would surface an incorrect value to end user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you ! In that call, I am wondering whether we can subtract the value of the outputs from some channel_value_satoshis
value and get the total fee that way... will think more this afternoon !
lightning/src/ln/channel.rs
Outdated
total_fee_sat, | ||
local_balance_before_fee_msat, | ||
remote_balance_before_fee_msat: _, | ||
} = SpecTxBuilder {}.build_commitment_stats(true, commitment_feerate, MIN_AFFORDABLE_HTLC_COUNT, value_to_self_msat, push_msat, &channel_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: here and a few others, move parameters onto newlines to save us needing to rustfmt them later
lightning/src/ln/channel.rs
Outdated
if holder_balance_msat < buffer_fee_msat + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { | ||
let stats = self.build_commitment_stats(funding, true, true, Some(feerate_per_kw), Some(htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize)); | ||
let holder_balance_msat = stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat; | ||
// Note that `stats.total_fee_sat` now accounts for any HTLCs that transition from non-dust to dust under a higher feerate (in the case where HTLC-transactions pay endogenous fees). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iiuc previously we were using the old commitment feerate to build our commitment transaction (and thus possibly not trimming some HTLCs that became dust), and now we're using the new proposed feerate so we'll properly trim them to dust and account for them in our fees?
Would be nice to have a test before this commit that demonstrates the htcs not being properly trimmed, and then updated in this commit to show that they are.
lightning/src/ln/channel.rs
Outdated
funding.get_channel_type(), | ||
); | ||
|
||
// Note that this is now the capacity available after we have subtracted any non-zero-value anchors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(very) nitpicky: remove "this is now" from comment, here and above. Somebody reading this code from fresh won't be looking at the diff, so the comparison to the past state doesn't help them (commit message seems like a a better home for notes on what's changed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly I tend to annotate my code for reviewers using comments like these :) Will move this to the git message thank you
lightning/src/ln/channel.rs
Outdated
// We don't include AwaitingRemoteRevokeToRemove HTLCs because our next commitment | ||
// transaction won't be generated until they send us their next RAA, which will mean | ||
// dropping any HTLCs in this state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Move comment up to above the if-let? Bit out of place here
lightning/src/ln/channel.rs
Outdated
let extra_htlc_commit_tx_fee_sat = TxBuilder::build_commitment_stats(&SpecTxBuilder {}, funding.is_outbound(), excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, 0, 0, funding.get_channel_type()).total_fee_sat; | ||
|
||
let htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); | ||
let commit_tx_fee_sat = TxBuilder::build_commitment_stats(&SpecTxBuilder {}, funding.is_outbound(), excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs, 0, 0, funding.get_channel_type()).total_fee_sat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder whether we've got the right API here when we're making calls like this passing zero values for local/remote amounts (and passing a feerate that we don't need in another place).
Did you consider splitting some of the components of CommitmentStats
out into their own methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I did, I agree with you I am not thrilled about this - we had a back and forth with Matt on this issue, will refresh on this this afternoon :)
@@ -5102,9 +5132,15 @@ where | |||
fn next_local_commit_tx_fee_msat( | |||
&self, funding: &FundingScope, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>, | |||
) -> u64 { | |||
let context = &self; | |||
let context = self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary &
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see seems this removes the &
?
lightning/src/ln/channel.rs
Outdated
funding.value_to_self_msat.saturating_sub(local_htlc_total_msat), | ||
(funding.get_value_satoshis() * 1000).checked_sub(funding.value_to_self_msat).unwrap().saturating_sub(remote_htlc_total_msat), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull these out into their own vars here and in next_local_commit_tx_fee_msat
to help readability a bit?
lightning/src/ln/channel.rs
Outdated
let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); | ||
let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); | ||
let mut nondust_htlcs: Vec<HTLCOutputInCommitment> = Vec::with_capacity(num_htlcs); | ||
let mut value_to_self_msat_offset = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking/personal preference: WDYT about tracking value_to_self_claimed
and value_to_remote_claimed
as separate u64
values and then used checked_sub
below? For the sake of avoiding negative numbers / slight readability improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great thank you a prior goal of mine was to remove all the raw as
casts here this one was the last one remaining !
…Builder` This is a temporary type that will be removed once we let `TxBuilder` choose which HTLCs are dust and which are non-dust.
In addition to the amount set by `commit_tx_fee_sat`, the transaction fee of a commitment transaction may also increase due to HTLC amounts that get burned to fees, and any elided anchors. We now include these amounts in the `transaction_fee_satoshis` field of `Balance::ClaimableOnChannelClose` too. This commit also makes the `transaction_fee_satoshis` field correct for custom transactions not encompassed in `chan_utils::commit_tx_fee_sat`.
3f98b95
to
a9892a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Major changes:
- Split out
build_commitment_stats
, see 660b86e - Introduce
IncludedHTLC
type to enforce the dust status of a HTLC onTxBuilder
without specifying the particular index, see f736d90- We will let
TxBuilder
set this status in the next version of this API, for now we focus on its use in zero-fee commitments.
- We will let
Balance::ClaimableOnChannelClose.transaction_fee_satoshis
now includes the sum of the fee due to dust HTLCs, and non-dust HTLCs getting rounded down, see a9892a9- Introduce standalone commit to remove the last raw cast in
build_commitment_stats
, see e26b232
Remaining TODOs:
- Append a commit to remove all
rustfmt::skip
in the code we touched. - Write a test to demonstrate how we were previously not accounting for the decrease in weight due to newly trimmed HTLCs when checking whether we can afford a new feerate in
can_send_update_fee
- Proper git messages
let extra_htlc_commit_tx_fee_sat = SpecTxBuilder {}.commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); | ||
let extra_htlc_htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: do we expand the commit_tx_fee_sat
call to also pass information back about the fees on HTLC transactions ?
hi @TheBlueMatt on this branch here i make an attempt at an API that supports "fixed additional outputs" on commitment transactions.
I place today's 330 sat anchors in that bucket, but the API should allow for custom ones.
The API should also let people customize all the existing outputs on the commitment transaction.
Still need to think about any remaining TODOs for additional outputs that are only present for some commitments (eg. DLC outputs), depending on how we'd like to prioritize these.